Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Updates to package.py #291

Merged
merged 6 commits into from
Aug 14, 2023
Merged

Updates to package.py #291

merged 6 commits into from
Aug 14, 2023

Conversation

mauneyc-LANL
Copy link
Collaborator

PR Summary

  • Add variants for spiner
  • Refine some depends_on to not grab unnecessary packages
  • Make sure CMake args are correct

@dholladay00 @jhp-lanl @jonahm-LANL @rbberger
Comments/merges/changes. We can discuss more during scheduled meetings.

PR Checklist

  • Adds a test for any bugs fixed. Adds tests for new features.
  • Format your changes by using the make format command after configuring with cmake.
  • Document any new features, update documentation for changes made.
  • Make sure the copyright notice on any files you modified is up to date.
  • After creating a pull request, note it in the CHANGELOG.md file
  • If preparing for a new release, update the version in cmake.

@mauneyc-LANL mauneyc-LANL self-assigned this Aug 9, 2023
@Yurlungur
Copy link
Collaborator

This all seems reasonable to me. Thanks for attacking this so quickly

@mauneyc-LANL
Copy link
Collaborator Author

This all seems reasonable to me. Thanks for attacking this so quickly

@Yurlungur I'd like to have a discussion/input on this before moving forward. Since we have a meet scheduled for day I think that'd be a good place to hammer this into shape.

@Yurlungur
Copy link
Collaborator

Yep sounds good

@jhp-lanl
Copy link
Collaborator

jhp-lanl commented Aug 9, 2023

This all seems reasonable to me. Thanks for attacking this so quickly

To echo Chris, yes please don't merge this yet. I plan to provide some input too

@mauneyc-LANL mauneyc-LANL marked this pull request as draft August 9, 2023 17:27
@jhp-lanl
Copy link
Collaborator

jhp-lanl commented Aug 9, 2023

Another issue I just found when trying to link with EAP: Cuda gets turned on by default in the CMake when Kokkos is on. I think there isn't logic to allow building with Kokkos but without Cuda in the package.py right now. Please correct me if I'm wrong though

@Yurlungur
Copy link
Collaborator

You can do -DSINGULARITY_ENABLE_KOKKOS=ON -DSINGULARITY_ENABLE_CUDA=OFF to override the cmake dependent option default settings. But I think the logic here probably should be reversed.

@jhp-lanl
Copy link
Collaborator

jhp-lanl commented Aug 9, 2023

You can do -DSINGULARITY_ENABLE_KOKKOS=ON -DSINGULARITY_ENABLE_CUDA=OFF to override the cmake dependent option default settings. But I think the logic here probably should be reversed.

I'm fine with either to be honest as long as the hooks are there to ensure we can build without Cuda both through CMake (exists) and through spack. I think that logic doesn't exist in the package.py yet.

@mauneyc-LANL mauneyc-LANL marked this pull request as ready for review August 9, 2023 22:39
jhp-lanl
jhp-lanl approved these changes Aug 10, 2023
View reviewed changes
Copy link
Collaborator

@jhp-lanl jhp-lanl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last request:

Can you update the build documentation for a few things:

  1. Change "Overview" to something about building. "Overview" becomes the page title and it's a bit too broad.
  2. Change spiner to an optional dependency and add something like "Optional; used for EOS that rely on databox or spiner"
  3. Move SINGULARITY_BUILD_CLOSURE to the standard options rather than dependent options
``SINGULARITY_BUILD_CLOSURE``           ON       Build the mixed cell closure models
  1. Reflect the fact that SINGULARITY_USE_KOKKOSKERNELS depends on SINGULARITY_BUILD_CLOSURE and not the other way around

Approving now so I don't forget to approve later

Yurlungur
Yurlungur approved these changes Aug 11, 2023
View reviewed changes
Copy link
Collaborator

@Yurlungur Yurlungur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved pending @jhp-lanl 's additional requests

@mauneyc-LANL
Copy link
Collaborator Author

@jhp-lanl @jonahm-LANL Docs updated

@jhp-lanl
Copy link
Collaborator

@dholladay00 or @rbberger if you have any comments can you make them by the end of today? Otherwise if not I think we can merge this.

rbberger
rbberger approved these changes Aug 14, 2023
View reviewed changes
Copy link
Collaborator

@rbberger rbberger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jhp-lanl
Copy link
Collaborator

I'll go ahead and merge this in. If there are any changes you want @dholladay00 we can open up some issues

@jhp-lanl jhp-lanl merged commit 9f3c8d6 into main Aug 14, 2023
@jhp-lanl jhp-lanl deleted the mauneyc/fix-package-depends branch August 14, 2023 23:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jhp-lanl jhp-lanl jhp-lanl approved these changes

@rbberger rbberger rbberger approved these changes

@Yurlungur Yurlungur Yurlungur approved these changes

@dholladay00 dholladay00 Awaiting requested review from dholladay00

Assignees

@mauneyc-LANL mauneyc-LANL

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mauneyc-LANL @Yurlungur @jhp-lanl @rbberger